Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

main,osbuildprogress: add --progress=text,debug support (COMPOSER-2394) #743

Merged
merged 8 commits into from
Dec 19, 2024

Conversation

mvo5
Copy link
Collaborator

@mvo5 mvo5 commented Dec 4, 2024

This adds a new progress flag that makes use of the
osbuild jsonseq progress information to show progress and
hide the low-level details from the user.

Here is what it looks like:
https://asciinema.org/a/cJpB6066UoDDzAylwHVHjEEIN

(c.f. COMPOSER-2394)

@mvo5 mvo5 force-pushed the progress3 branch 2 times, most recently from d31e3d4 to c467ef0 Compare December 5, 2024 10:07
@mvo5 mvo5 marked this pull request as ready for review December 5, 2024 10:07
@ochosi ochosi changed the title main,osbuildprogress: add --progress=text,debug support main,osbuildprogress: add --progress=text,debug support (COMPOSER-2394) Dec 9, 2024
@mvo5 mvo5 force-pushed the progress3 branch 11 times, most recently from 503f610 to 313b4f7 Compare December 11, 2024 11:07
@mvo5 mvo5 marked this pull request as draft December 11, 2024 11:15
@mvo5
Copy link
Collaborator Author

mvo5 commented Dec 11, 2024

Moved back to draft as the "pb.Pool" idea does not work with podman as it does not give us a real pty

@mvo5

This comment was marked as outdated.

@mvo5 mvo5 marked this pull request as ready for review December 11, 2024 17:03
Copy link
Member

@ondrejbudai ondrejbudai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is awesome, I only found some issues mostly with comments and method names. PTAL :)

And then ofc we need to discuss the whole --log-driver=passthrough-tty matter. :)

bib/internal/progress/progress.go Outdated Show resolved Hide resolved
bib/internal/progress/progress.go Show resolved Hide resolved
bib/internal/progress/progress.go Outdated Show resolved Hide resolved
bib/internal/progress/progress.go Outdated Show resolved Hide resolved
bib/internal/progress/progress.go Outdated Show resolved Hide resolved
bib/internal/progress/progress_test.go Outdated Show resolved Hide resolved
bib/internal/progress/progress_test.go Outdated Show resolved Hide resolved
bib/internal/progress/progress.go Outdated Show resolved Hide resolved
bib/cmd/bootc-image-builder/main.go Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
This commit adds a progressbar interface and implementations:
1. terminalProgressBar based on github.com/cheggaaa/pb/v3
2. plainProgressBar to just pass the message but no progress
3. debugProgressBar to print the osbuild/images status

The interface is very tailored to the needs of bib but it should
be similar enough to be reusable by `image-builder-cli`.

It contains a top level spinner that gives the current step:
"Make manifest" or "Building image". The the subprogress from
osbuild: the pipelines and the stages from the pipelines as
progress bars and the last line is the latest status message,
usually something from osbuild like "Starting pipeline qcow2".

The spinenr is a bit of a creative choice, it could also be a
progress bar because we know the steps (manifest+build). But the
runtime is totally dominated by the build step so having a first level
progress bar is odd as the UI will spend 90% of the time in it.
Plus the spinner gives it more a more "dynamic" appearnce.
@mvo5 mvo5 force-pushed the progress3 branch 2 times, most recently from 33403b4 to 125407a Compare December 12, 2024 18:40
@mvo5 mvo5 force-pushed the progress3 branch 2 times, most recently from c620cd6 to 6c1dd02 Compare December 12, 2024 19:36
@ondrejbudai ondrejbudai added this pull request to the merge queue Dec 18, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 18, 2024
@ondrejbudai ondrejbudai added this pull request to the merge queue Dec 18, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Dec 18, 2024
@mvo5 mvo5 added this pull request to the merge queue Dec 18, 2024
@mvo5 mvo5 removed this pull request from the merge queue due to a manual request Dec 18, 2024
@mvo5 mvo5 requested a review from ondrejbudai December 18, 2024 13:04
mvo5 added 6 commits December 18, 2024 17:43
This commit implements enough of `pb.Pool` for our needs
without the need to implement raw terminal access.

It turns out that by default podman does not connect the
full tty, even in `--privileged` mode. This is a sensible
security default but it means `pb.Pool` does not work as
it wants to set the terminal into "raw" mode and will fail
with an ioctl() error when not running on a terminal.

However we really just need simple ANSI sequences to
render the pool so this seems unnecessary. The initial
idea was to just use  `--log-driver=passthrough-tty` but
that is not available on podman 4.x. (which is part of
Ubuntu 24.04 LTS and the GH actions).

So this commit just implemnts a custom pool like renderer.
The Start/Stop methods can no longer error so remove the error
from the signature. The commit also tweaks the doc strings and
comments slightly.
manifestFromCobra generate an osbuild manifest from a cobra
commandline and it also takes a progress.ProgressBar as input.

It needs an unstarted progres bar and will start it at the right
point (it cannot be started yet to avoid the "podman pull" progress
and our progress fighting). The caller is responsible for stopping
the progress bar (this functtion cannot know what else needs to happen
after manifest generation).

TODO: provide a podman progress reader to integrate the podman progress
into our progress.

Thanks to Ondrej for the suggestion to clarify the relationship
further.
This commit adds some extra checks to ensure that the pb.ProgressBar
does not contain any errors. This should not happen but we want
to be paranoid (especially since the error handling of pb.ProgressBar
is a bit indirect).
This commit fixes a bug in the progress rendering when no terminal
width can be identified. In this case pb.ProgressBar will strip
bars with dynamic elements. This is undesirable so when no size
can be found we just use the pb.defaultBarWidth (which is not
exported sadly). This should get fixed upstream ideally.
The upload code is currently using a plain "pb.ProgressBar" and
this is tested as part of our integration test. This will collide
with our own progress implementation. For now workaround this by
stopping our own progress and let plain "pb" takeover.

I was tempted to fix it right away but the PR with the progress
is already relatively big so I opted for a followup.
// *for now* just stop our own progress and let the uploadAMI
// progress take over - but we really need to fix this in a
// followup
pbar.Stop()
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is addressed in #763

Copy link
Member

@achilleas-k achilleas-k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome stuff. Thank you!

@mvo5 mvo5 enabled auto-merge December 18, 2024 17:34
@mvo5 mvo5 added this pull request to the merge queue Dec 18, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 18, 2024
@mvo5 mvo5 added this pull request to the merge queue Dec 18, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 18, 2024
@mvo5 mvo5 added this pull request to the merge queue Dec 19, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 19, 2024
@mvo5 mvo5 added this pull request to the merge queue Dec 19, 2024
Copy link
Member

@ondrejbudai ondrejbudai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Merged via the queue into osbuild:main with commit 8f3ca27 Dec 19, 2024
10 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants